Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix comment nodes being specified as children in a satisfy spec. #219

Merged
merged 3 commits into from
Oct 2, 2018
Merged

Fix comment nodes being specified as children in a satisfy spec. #219

merged 3 commits into from
Oct 2, 2018

Conversation

alexjeffburke
Copy link
Member

@alexjeffburke alexjeffburke commented Sep 29, 2018 •
edited
Loading

This is a backwards compatible version of the previous PR that ensures that any DOMNode present in the children property of an object spec is converted before comparison.

Fixes #217.

@alexjeffburke alexjeffburke mentioned this pull request Sep 29, 2018
Closed
@coveralls
Copy link

coveralls commented Sep 29, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 822

  • 7 of 8 (87.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.06%) to 91.499%
Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/index.js 7 8 87.5%
Totals Coverage Status
Change from base Build 808: -0.06%
Covered Lines: 525
Relevant Lines: 555
💛 - Coveralls

@sunesimonsen
Copy link
Member

Mhh I never thought of using ignore in an object spec context, in that case you can just use expect.it. Can you point me to the problem in unexpected-reaction. The ignore comment was just syntactic sugar for ignoring with expect.it.

@alexjeffburke alexjeffburke mentioned this pull request Sep 29, 2018
Open
@alexjeffburke
Copy link
Member Author

@sunesimonsen I've documented it here: unexpectedjs/unexpected-reaction#5. Let me know what you think :)

sunesimonsen
sunesimonsen reviewed Sep 29, 2018
View reviewed changes
lib/index.js Outdated
value.children
Array.isArray(value.children)
? convertChildrenToSatisfySpec(value.children, isHtml)
: value.children
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we always convertChildrenToSatisfySpec, what if I'm providing a DOM tree as a single items? Maybe I don't have the full overview 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe so - based on my understanding of the rest of the assertions, I believe it is possible for a DOMNodeList object to be supplied as the value of children and the comparison should still work. If individual DOM nodes are supplied they'd be in an array which would need the conversion :)

sunesimonsen
sunesimonsen reviewed Sep 29, 2018
View reviewed changes
Copy link
Member

@sunesimonsen sunesimonsen left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that I should probably have written more tests for ignore, it was sort of an experiment that got shipped to production - as all prototypes :-)

Sorry about that.

@sunesimonsen
Copy link
Member

It would be great to get @papandreou take a lot at this as well. I'll take a deeper look at it tomorrow.

@alexjeffburke
Copy link
Member Author

This is definitely the one to consider.

What this change specifically allows is full DOM nodes to be supplied as the elements of the children array. It runs them through a conversion to satisfy specs which means all the matching done by that function gets picked up (for example the expect.it('to be an', 'DOMNode') in the case of a DOMCommentNode containing ignore).

It also maintains the invariant of plain strings resulting in a text based comparison of the nodes which should mean that it is backwards compatible, something I did not intend to undermine.

@papandreou
Model <!--ignore--> as its own type and implement the ignoring via an…
fe9ce65 
… assertion

Fixes <!--ignore--> as the top-level to satisfy RHS.
@papandreou papandreou force-pushed the fix/comment-nodes-in-children-satisfy-spec branch from fe9ce65 to 25bd2b4 Compare September 30, 2018 20:30
@papandreou
Copy link
Member

I'd like to get <!-- ignore --> to work in all situations, not just when you satisfy against <object>. Added some more tests and got it to work by adding a new type: #220

@sunesimonsen
Merge pull request #220 from unexpectedjs/fix/top-level-ignore
006c7ec 
Model <!--ignore--> as its own type and implement the ignoring via an assertion
@sunesimonsen sunesimonsen merged commit 74c1eef into master Oct 2, 2018
@sunesimonsen sunesimonsen deleted the fix/comment-nodes-in-children-satisfy-spec branch October 2, 2018 17:56
@sunesimonsen
Copy link
Member

Released as [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunesimonsen sunesimonsen sunesimonsen left review comments

@papandreou papandreou Awaiting requested review from papandreou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexjeffburke @coveralls @sunesimonsen @papandreou